Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add routing service #187

Closed
wants to merge 18 commits into from
Closed

Conversation

hgzimmerman
Copy link
Member

@hgzimmerman hgzimmerman commented Apr 6, 2018

Hey, this is in no way ready to merge, but I've been working on implementing routing in my own app and figured I could touch up my implementation and possibly have it merged, or at the very least, provide inspiration for a better routing service.

I think the core idea is good, but the way crate consumers interact with the service could use some improvement. An example has been added to demonstrate the service.

My general thoughts on things that should be improved appear in TODO comments.

These changes have been tested on Firefox 60 and Chromium 61 in Linux. The test application was built using cargo-web 0.5.0 with the asmjs target using my system installed emscripten. emcc's version is 1.37.16.

Things I would like to address:

  • How should errors be handled in the routing service? I currently use a lot of expect() and eprintln!() to keep the interface to the crate's users simple, but I'm not sure if that is the right approach in all cases here.
    • I think some stuff like window().location() returning None would indicate that the browser doesn't support that feature. How could this be exposed to users of the crate better than by just panic!()ing? ~~ Is a RoutingError enum in order?~~
    • Update: I still throw the occasional error, but I reduced the total number of them. I now expect users to route based on a Result<RouteInfo, RoutingError> that is returned from the routing callback.
  • By providing checks for strings that contain '/' for paths, I feel like I have over-complicated the way crate users specify what route they want to go to. I would like to relax, and come up with a better scheme for converting routes represented by enums into an acceptable path because currently, this doesn't work with routes with more than 2 path segments. I think I came up with a reasonable solution by using RouteInfos at the interfaces of the route service.
  • This PR drags in the url crate into Yew, bloating the JS/WASM file served to users, and currently leaks url::ParseError into Yew's exported types, which isn't acceptable.
    • Given how we are only ever dealing with the path, query, and fragment, it may make sense to handle String -> RouteInfo conversions ourselves and not use the url crate. Any errors resulting from parsing could fall under the aforementioned RoutingError emun.
    • Update: For now I've decided to use the url crate, although the interface presented to the crate user allows that to change in the future.
  • If we end up rolling our own route parser, would creating a route_info!() macro be a good idea to allows users to specify routes by running the parser at compile time and failing to compile if the provided &str is invalid? This would be way better than the current approach that involves bringing in every Route enum needed to construct your desired path and putting them in a vector that is used to construct the RouteInfo struct. (Update: The current approach is very amenable to conversion to this macro, ideally only moving from run-time to compile-time errors)
  • Any other feedback is appreciated.

@vitiral
Copy link
Contributor

vitiral commented Apr 6, 2018

haha, we both created routing services at identical times... let the battle commence! #185

Let's review eachother's code and discuss design :)

Edit: from a quick review it looks like your Service is much more "batteries included" and mine is much more "sparse simplicity". This is good! We should determine what is actually useful.

I also notice you included History fiddling, which I feel doesn't belong in a Service (but I can absolutely be wrong). I opened #186 to discuss. I'll look more at your code to see if I missed something.

@therustmonk
Copy link
Member

@vitiral @hgzimmerman It's amazing thing! There was no one router and there are two now 😲

Thank you, friends! 👍 Let's try to join the best of two ideas. I could review the both on this weekends.

Copy link
Contributor

@vitiral vitiral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I am comming to a design which is very similar to yours other than the points I laid out. Comments appreciated!

/// A subset of the url crate's Url object that can be passed
/// to crate consumers to deal with routing.
#[derive(Clone, PartialEq, Debug)]
pub struct RouteInfo {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I strongly prefer that this be only two items:

  • state: Option<Value>
  • location: Location

The Location type is then just the resolved items in:

  • js!{return window.location}
  • OR js!{return new URL(@{href.to_string()})}

i.e. we will handle any thrown SecurityErrors and panic/log there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main benefit is that we do not require an extra library, but also we are standards compliant with browsers.

Copy link
Member Author

@hgzimmerman hgzimmerman Apr 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree that RouteInfo should require holding onto a Location struct. I think that RouteInfo should only contain information essential to routing, which excludes information in the origin. This implies that it should contain

  • path: String or path_segments: Vec<String>
  • query: Option<String>
  • fragment: Option<String>
  • state: Option<T: JsSerialize>

I do agree that the Location API, once StdWeb adds support for it, should be used to get info about the current route during the handling of a PopStateEvent. I think my current approach is misguided in storing the route string in the history's state, and then parsing that into a RouteInfo. Instead, when the event is fired, I should get the RouteInfo from the location API (currently by parsing the href via url, in the future, by using stdweb's Location's fields), and try to attach the state if it was Some.

I still do want to use the url crate because my routing implementation uses it to validate routes passed to it by crate users, and I trust it to perform better than anything I could ever write.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, my push_state ended up looking like this:

    /// Set the state of the history, including the url.
    ///
    /// This will only trigger a state change in the frontend if `emit == false`
    pub fn push_state(&self, emit: bool, state: Value, title: &str, url: Option<&str>) {
        let route_fn = match self.route_fn {
            Some(r) => r,
            None => panic!("Attempted to set_state without initializing router"),
        };
        let url = match url {
            Some(url) => url.to_string(),
            None => self.window.location().unwrap().href().unwrap(),
        };
        self.history.push_state(state.clone(), title, Some(&url));
        if update {
            let info = RouteInfo {
                location: parse_location(&url),
                state: state,
            };
            self.callback.as_ref().unwrap().emit(info)
        }
    }

That parse_location basically just calls js!{ return new Url(url); }.

So... I ended up parsing the url anyway! I'm starting to think that doing it through a library is probably worth it.

This brings up an interesting point... none of what we are doing here needs to be in the yew crate itself. We could make our own yew_route crate! This would allow us to experiment and include whatever dependencies we might want.

Conclusion

I'm coming around to doing the url parsing in rust. I also agree that having type safety on the URL types (via a macro in the future) is worth pursuing.

However, I disagree with your layout. Why wouldn't RouteInfo just be two fields, state and url, where url is the Url type?

Copy link
Contributor

@vitiral vitiral Apr 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I want to make sure we support is the ability to set_state to a partial URL, i.e. just the hash/query.

Another possibility is to require the user to hold onto their initial Url "base" and use things like set_query to set the query they want, etc. I think this is actually much more clean, although possibly at the cost of some ergonomics (the user has to have access to their base url at all relevant times).

Setting the query based on a base-url is currently annoying, so I opened servo/rust-url#447

pub fn new() -> RouteService {
RouteService {
history: window().history(),
location: window().location().unwrap(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree 100% that we should unwrap here.

However, I think we should resolve the attributes by unwrapping them as well into a plain struct (panic for SecurityError).

/// The callback takes a string, parses it into a url, and then uses the result of that
/// to create a message that the component will use to update itself with.
// TODO I would like to include this in register_router
pub fn create_routing_callback<COMP, CTX>(context: &mut Env<CTX, COMP>) -> Callback<RouteResult>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be removed. We are just calling context.send_back, which is required for all services. I don't think this adds anything.

Copy link
Contributor

@vitiral vitiral Apr 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also disagree with the API of requiring the user to implement COMP::Msg::from(route_result). The callback can handle that by itself if we used it in the "normal" way: the user created the Callback using a function of type Fn(RouteInfo) -> Msg

Copy link
Member Author

@hgzimmerman hgzimmerman Apr 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined to agree. I wanted the code within create_routing_callback() to live within register_router() but because I couldn't both mutably borrow the context to get access to the router in order to call that function, as well as pass a mutable reference to the context as a parameter to that same function, I ended up creating this awkward function that needs to be called beforehand.

Because everything else requires calling context.send_back(), I don't see the problem with removal of this.

I personally like using the From and Into traits to structure my code, but I can understand that for some simple apps that only have static routes, it makes sense to just define all routing in one place, and the most logical place to do that is in the callback.

/// Registers the router.
/// There can only be one router.
/// The component in which it is set up will be the source from which routing logic emanates.
pub fn register_router(&mut self, callback: Callback<RouteResult>)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I chose to name this initialize since it fits with yew::initialize().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initialize() works, but I chose register_router() because this isn't called at app start.

Conceptually, someone could write an app that has a bunch of possible states where the component that performs routing isn't even instantiated until they perform some task. This scenario may be inadvisable, but Yew currently allows for it. Because of this, the router component could be instantiated and destroyed multiple times.

I feel like there is a significant functional difference between yew::initialize() and how the router hooks into a component to justify a different name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the RoutingService is attached to the Model by convention instead of the Env then we could accomplish everything within a single new() in the Model::create method. Then we wouldn't have these annoying Option types.

It also allows people to call set_state (or whatever) in the Model::view() method, which allows for buttons, etc to respond directly. Considering that you should not call Callbacks within update I think this is practically a requirement.

What do you think?


/// Sets the route via the history api.
/// This does not by itself make any changes to Yew's state.
fn set_route(&mut self, route_info: RouteInfo) -> RouteDidChange {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is where things get interesting.

The javascript form of this history.pushState which accepts:

  • the state object (arbitrary javascript value)
  • a "title", not currently used
  • a URL.

I think we should do the same. Requiring someone to construct route_info is unergonomic IMO.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The set_route method is actually the main point of difference between our APIs, and I am convinced mine is better.

In my implementation it returns COMP::Msg. The reason it does this is to allow the user to decide whether to propagate the "route change event". For instance, if you did set_route in response to a button click you could choose whether to send the resulting message or send a different one. That is very intuitive IMO.

To do this all it has to do is call the wrapped Fn in the context, which we need #188 for.

Copy link
Member Author

@hgzimmerman hgzimmerman Apr 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To address your first comment regarding the ergonomics, yes, creating RouteInfo structs is a burden placed upon the crate user, but the alternative is to pass strings around, which can lead to bugs at runtime. For example, the function responsible for mapping route strings to component properties could recieve a string that wasn't correctly formatted because the developer concatenated "/parent/" and /child together to create /parent//child and tried to use it for routing, when their mapping function expects /parent/child. By forcing users to parse their strings into RouteInfo, these errors can be prevented.

In the spirit of Rust, I'm trying to make what could otherwise be a runtime error into a compile time error. I don't want the route service's functions to be "smart" and insert extra leading '/'s or convert occurrences of // to /. I want those error cases to either not be possible, or throw compile time errors, or throw runtime errors the crate user could choose to handle as they wish, before routing and history/url manipulation even occurs.

Currently I have a pattern like:

Route::DogForum => RouteInfo::parse("/dog").unwrap(),

which, yes, could throw a runtime error.

I intend to move this to something like:

Route::DogForum => route_info!("/dog"),

which would invoke the RouteInfo's parse function at compile time, and stop compilation if the parser couldn't convert it to a RouteInfo. This would imply that the parse function used in the macro could only take &'static str.

It would also look like the following for when the user wants to switch routes.

context.routing.call_link(route_info!("/parent/child"));

If you wanted to ensure that you only call currently routable links and defend against the possibility of the route strings to specific components changing and all "links" to them no longer referring to that component:

context.routing.call_link(component_1::Route::Parent.into() + component_2::Route::Child.into());

or in the case of wanting to attach dynamic information, something like this could be used:

// parse_relaxed() could be used to parse strings that possibly don't have a leading slash.
match RouteInfo::parse_relaxed(some_user_specified_value.to_string()) {
    Ok(dynamic_route_info) => {
        let route_info = route_info!("parent/child") + dynamic_route_info;
        context.routing.call_link(route_info)
    }
    Err(e) => eprintln!("Dynamic route could not be parsed")
}

I think this extra ceremony for creating navigable information provides safety benefits that outweigh the concerns about verbosity and ergonomics.

/// Sets the route via the history api.
/// This does not by itself make any changes to Yew's state.
fn set_route(&mut self, route_info: RouteInfo) -> RouteDidChange {
if route_info != self.get_current_route_info() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the spec actually allows you to do this, mainly to update the state (I think)

The new URL can be any URL in the same origin as the current URL.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I store the current route string as the state. I don't know if this is standard practice, or if in other routers they store the route as well as (part of?) the model. Conceptually you could restore the routed-to model's data on a back event using a tuple of (String, Option<Model>) where Model implements Storable, I decided pretty early on that I only wanted to work with Strings to keep my implementation simple. Pre-Edit: I wrote the above text before I came to the conclusion that I actually want the ability to store arbitrary state in the History API. It no longer applies.

On to addressing your comment:

I specifically did not want it to be possible to add more history entries to the same page you are already on. Just like how a plain HTML page won't reload the page when you click a link that points to that same page, I don't want webapps built using Yew to be able to add history entries by clicking a link that appears in a header 30 times in a row. If they did that without that conditional, they would have to press the browser's back button 30 times to go to a prior page on the site.

The principle of least surprise indicates that Yew should prevent consecutive duplicate entries to the History API.

@hgzimmerman
Copy link
Member Author

For all of the original points of concern I brought up, I think I've gotten to a point where I'm comfortable with them. If something isn't inherently clear, it can be addressed in a guide of sorts (eg, best practices for using routing with the fetch service).

What I'm still struggling with is the interface presented to the users of Yew. Currently I have the crate user implement From<&'a RouteInfo> for user_defined::Route and Into<RouteInfo> for user_defined::Route in order for them to easily interface with the router service. This allows crate users to manually determine exactly how route path segments should correspond to the enums that they attach to Props for their components.

This allows flexibility in the case of dynamic paths. For example if instead of the forums::Route being defined as:

pub enum Route {
    CatForum,
    DogForum,
    ForumsList
}

instead it was:

pub enum Route {
    Forum(String),
    ForumsList
}

The crate user can choose to map a path to the forum like so:

match path_segment {
    "cat" => return Route::Forum("cat".to_string()),
    "dog" => return Route::Forum("dog".to_string()),
    _ => return Route::ForumsList
}

or even more dynamically:

if path_segment == "" {
    return Route::ForumsList;
} else {
    return Route::Forum(path_segment.to_string());
}

The problem I have with this current approach is that it is a lot of work for the user to perform per routable component, and I think it can be made to be even less.

@deniskolodin I would like your thoughts about the following proposal. Is this a direction worth moving in?

Using a proc_macro, it should be possible to define attributes that allow an Enum to convert to and from a a path string.
Something like:

// === Forums.rs ===
#[derive(Routable)]
pub enum Route {
    // Instructs the current segment to be stored in the String, as well as to resolve the forum::Route from further path segments
    #[route(path = String)] 
    Forum(String, forum::Route),
    #[route(path = "")]
    ForumsList,
}

By deriving Routable, it would specify that all enum variants require an attribute marking them so that it knows how convert them to and from path strings.
The #[route(path = String)] could convert to the path string to the wrapped type, of course, after trying to match the ones tagged with #[route(path = "something_else")].

In the case where you have a routing component that handles page not found errors it could look like:

// === Main.rs ===
#[derive(Routable)]
enum Route {
    #[route(path = "forums")]
    Forums(forums::Route),
    #[route(path = "login")]
    Login,
    // It will only route to this path if an error is encountered, but will set the url to "pagenotfound" if it does
    #[route(error, path = "pagenotfound")]
    PageNotFoundRoute
}

If any route at any level of routing couldn't be resolved, an error could be propogated upwards through the path-resolving hierarchy until it encounters an enum variant that is tagged with #[route(error)] and the path will resolve to that.

Enums that derived Routable would have TryFrom<&'a RouteInfo> and Into<RouteInfo> implemented for them.

The concerns about this approach are that proc_macros require nightly for now and that excessive use of macros may bloat compile time.
Also, I'm not exactly sure if rust's macro system allows for this aforementioned example work:

    #[route(path = String)] 
    Forum(String, forum::Route),

On the other hand, this doesn't require significant changes to my current design, this merely acts as sugar on top of the existing proposal.

@therustmonk
Copy link
Member

@hgzimmerman Thank you for this PR. It's inspiring.

Routing is a hard to implement feature. Yew looks like React and Elm, but actually has other architecture/implementation, because I couldn't transfer usual approaches to Rust and had forced to reinvent it.

I think we should move the same way with a routing. We should reinvent it again. Sounds strange, but I'll try to dream up: we could use traits to make components Routable and derive routing paths and behaviour automatically. I think it's a good challenge for a language like Rust. You do a good experiments and we should move them forward to the desired state.

@vitiral
Copy link
Contributor

vitiral commented Apr 12, 2018

@deniskolodin interesting... components as the primary endpoint of routes...

I think this missses that there can be a lot of information within the route. I feel that a Msg is the correct result of a route, correct? For instance, someone might have a specific query or hash -- handled by a component (maybe...) but has more info.

Or are you thinking of doing a component+msg... or something?

I feel like adding complexity here is a mistake. Fortunately it has been proven that a "simple" router can easily be an external crate -- so by all means we can try multiple solutions (including one supported in core). Regardless I think it's a good idea to try out a lot of soutions. Please include me on the review!

@hgzimmerman
Copy link
Member Author

hgzimmerman commented Apr 13, 2018

Thanks Denis.

I've been dogfooding my ~2.5kloc Yew-based project with the routing implementation in this PR and I have a few observations:

  • It works very well with static routes.
  • As soon as I throw a dynamic route at it, a mismatch between routing and data appears. The obvious by default pattern is to have the parent resolve the route of the child and then instantiate the child component via the view. This doesn't have great ergonomics when you need to set the path based on the state of the child of the component responsible for setting the path.
  • Using this solution is still very verbose, as using the router requires specifying both "from route" and "to route" functions that usually contain the same data, just moving in "opposite directions".

I think that for any robust routing system, the mechanism that performs routing needs to operate on a tree (either defined at compile time, or dynamically). Defining how that tree is constructed and the rules for its resolution from a string are the responsibility of the router authors.

With your suggestion that we attempt to reinvent routing, binding the routing logic to the component/model, the following is the best proposal I could come up with in a short period of time.

First, I suggest we move away from defining routing enums that dictate which child component gets rendered by the routing component.
Instead, any component that can be routed to defines the conditions on which it will be routed to (route_to), and what its route will be(to_route). These can be different for catch-all cases like "PageNotFound" pages, but for static routes, the path section string should always be the same, and hence those two functions could be covered by a proc_macro_derive.
Then, that component's parent could register with the router service what children it could possibly route to. When routing, the router would iterate through the path_segments vector, and for each segment it would try to resolve which child it could instantiate based on the child's "route_to" function.
In the render function of the parent, some syntax could be added to allow Yew to insert the child at that location.

This would allow Yew to dynamically create a decision tree where each node in the tree can decide for itself if it gets rendered or if its parent node moves on to try to match against the next possible component.

In all, some of the magic bits would have to be ironed out, but I think it could look roughly like;

struct ParentModel {
    ...
}

impl Component<Context> for ParentModel {
    fn create(props: Self::Properties, context: &mut Env<Context, Self>) -> Self {
        // Some macro magic may be required for this to work
        context.routing.register_routes(vec![ChildModel::from_route, PageNotFoundModel::from_route]); // Ordering is significant
        ...
    }
    ...
}

impl Renderable<Context, ParentModel> for ParentModel {
    fn view(&self) -> Html<Context, Self> {
        html!{
            <YEW_ROUTER></YEW_ROUTER> // Yew would insert the resolved child here
        }
    }
}


////////////////////////////

#[derive(Routable)]
#[path = "child"] // Will only match if the path segment == "child"
pub struct ChildModel {
    ...
}

impl Component<Context> for ChildModel {
    ...
}

impl Renderable<Context, ChildModel> for ChildModel {
    ...
}


//////////////////////////////

pub struct Props {}

pub struct PageNotFoundModel {
    ...
}

impl Component<Context> for PageNotFoundModel {
    ...
}

impl Renderable<Context, PageNotFoundModel> for PageNotFoundModel {
    ...
}

impl Routable for PageNotFoundModel {
    // The Option return type would indicate if the route matched or not.
    // If it did match, then the props are passed to the create function
    fn from_route(_route: RouteInfo) -> Option<Props> {
        Ok(Props {})
    }
    fn to_route(&self) -> RouteInfo {
        route_info!("/PageNotFound")
    }
}

With this approach, I am worried about the possibility of re-creating the whole hierarchy of routable components when any part of the route changes. I don't know much about Yew's internals, but maybe in the section of code that resolves the route, it could check if the component corresponding to the props returned from from_route exists or not and depending on that, either destruct the prior component and instantiate the new one or just pass the props to the existing component's change() method.

This also has the problem of tightly coupling the router to Yew via the proposed specific routing "html!" syntax, making extracting it to another crate next to impossible.

@hgzimmerman
Copy link
Member Author

hgzimmerman commented Apr 14, 2018

I've been thinking about the proposed alternative from above for a few days and I think I came up with something better.

To avoid some of the magic and tight coupling, as well as removing some ambiguity on how the accidental recreation problem can be avoided, I propose the following.

The Routable traits will look approximately like

/// The component that implements this trait shouldn't correspond to any route itself,
/// but does have children that the router will need to resolve. 
/// When the router determines which child to route to, set_child() will be called and
/// will update the child field of the Model. This should be easily deriveable in most cases.
/// There should only be one Model that implements this trait, as it will be the start of where routing occurs.
trait RoutableRoot {
    fn set_child<T>(&mut self, T) 
        where T: RoutableChild + Component<??> + Renderable<??>;
}

/// This trait should be implemented or derived for Models that sit below the RoutableRoot,
/// but have children themselves.
trait RoutableNode {
    // RoutePath is just an alias to string
    fn from_route(route: RoutePath) -> Option<Props>;
    fn to_route(&self) -> RoutePath;
    fn set_child<T>(&mut self, T) 
        where T: Component<??> + Renderable<??>;
}

/// The router will want to be able to resolve the component that implements this, 
/// but because it doesn't have any children, there doesn't need to be any set_child function.
trait RoutableLeaf {
    // RouteLeaf is a struct that contains the last segment from a vec of path segments,
    // as well as options of both fragment and query.
    fn from_route(route: RouteLeaf) -> Option<Props>;
    fn to_route(&self) -> RouteLeaf;
}

The router service would break down the existing RouteInfo structs into component RoutePaths and RouteLeafs via iteration and attempt to resolve different Routable components based on them.

The ChildModel would remain similar to implementation shown in my prior comment. For the ParentModel, it would now look like:

#[derive(RoutableRoot)]
struct<T> ParentModel 
    where T: RoutableChild // Marker trait applied to both RoutableNode and RoutableLeaf (?)
        + Component<??>
        + Renderable<??>
{
    #[child]
    child: T
}

impl Component<Context> for ParentModel {
    fn create(props: Self::Properties, context: &mut Env<Context, Self>) -> Self {
        // Some macro magic may be required for this to work
        let routes = magic_macro!(ChildModel, PageNotFoundModel]); // Ordering is significant

        // This now takes a reference to &mut self that it uses to create a callback that will 
        // execute set_child() when it determines a child route.
        context.routing.register_routes(routes); 
        ...
    }
    ...
}

impl Renderable<Context, ParentModel> for ParentModel {
    fn view(&self) -> Html<Context, Self> {
        html!{
            {self.child.view()}
        }
    }
}

The router service, using information from magic_macro!, could generate one of many possible Props structs from the route info and instantiate the child component that corresponds to it. Then pass this component to the callback and call set_child() with it. (this can't be done with the current design because the register_routes() function as designed can't take a &mut reference to a self that doesn't exist yet, but I think this can be designed around using Tasks)

No idea if this is possible to implement (hence the ??s in the type signatures for Component and Renderable), but I think its a novel approach worth exploring.

@deniskolodin I would like some reassurance that this is the direction we would like to move routing with Yew towards before I start attempting to implement a routing backend that supports this sort of interface to the crate user.

EDIT:
In order to avoid the awkwardness with the proposed Marker trait, I could condense the traits into:

trait Router {
    fn set_child<T>(&mut self, T) 
        where T: Routable + Component<??> + Renderable<??>;
}

trait RoutableNode : Routable + Router; // Is this distinct trait even necessary anymore?

trait Routable {
    fn from_route(route: RouteLeaf) -> Option<Props>;
    fn to_route(&self) -> RouteLeaf;
}

@therustmonk
Copy link
Member

therustmonk commented Apr 19, 2018

@hgzimmerman Thank you for these experiments. The last concept come closer to a perfect solution.
I expect the routing should be a utility which you don't need to think about for joining.

For example:

// Somewhere in the framework
trait Routable {
    fn get_part(&self) -> String;
    fn tune_from_part(&mut self, path: &str) -> Result<(), Its404>;
}

impl<CTX, COMP> Scope<CTX, COMP> {
    pub fn mount_in_place(...) {
        self.fit_to_url_path(); // Impl depends on `COMP: Renderable`
    }
}

struct Its404; // Error
type UrlPart = String;

// In the app
enum Model {
    Scene1,
    Scene2,
}

trait Routable {
    fn get_part(&self) -> UrlPart {
        let part = match *self {
            Model::Scene1 => "scene1",
            Model::Scene2 => "scene2",
        };
        part.to_owned()
    }
    fn tune_from_part(&mut self, path: &str) -> Result<(), Its404> {
        match path {
            "scene1" => {
                *self = Model::Scene1;
                Ok(())
            },
            "scene1" => {
                *self = Model::Scene1;
                Ok(())
            },
            _ => {
                Err(Its404)
            },
        };
    }
}

Child components also have to play this rules.
Let's imagine the case when you could use even a complex regex to restore a state. I think it should be great.
... error handling still is a slimy question )

@hgzimmerman
Copy link
Member Author

hgzimmerman commented Apr 19, 2018

I did try to implement my 2nd/3rd suggestion but did run into problems regarding creating new components and trying to infer what a component is depending based on the props. That was a bad idea.

Provided the self.fit_to_url_path() hooks into yew's render cycle at the right point, then I see that suggestion as far superior to users having to define a set_child().

I do have some concerns about how this would work to users of Yew.

I think the component itself needs to be aware of the changes the router performs.
If a component makes a network request based on what the route is. eg: /users/34, you would want to expose the ability to make a request when the router detects the change.

This would involve 3 components, the main component, the users component (which could also just show a list of users), and the individual user viewer component. So the router would want to create sets of enums like: Model::User, UserModel::Individual(34) from the route string. Whenever the user id changes, you would want to cause the viewer component to request new user data to show from the server.

So directly mutating the component shouldn't be an option. Instead, you either want to allow the context in tune_from_part() so you can register callbackts to the fetch service or you return the child component's Properties from tune_from_part(), and mount_in_place adds the props to the updates vector. Then the user can do their networking in the change() method. This would require the Routable trait to look like:

trait Routable: Component<...> { // you can only implement Routable for things that already implement Component
    fn get_part(&self) -> String;
    fn tune_from_part(&mut self, path: &str) -> Result<Self::Properties, Its404>;
}

This creates additional questions. How do we determine if the route really did change for this component, and not some child component in the case of users/34/#settings? You wouldn't want to make network requests if you didn't have to.
Conceptually, if we retain the structured RouteInfo object, Yew, or possibly the user, could perform an equality check against get_part() and the new route segment, and if they are the same, allow the user to branch to avoid making network requests.

In regards to error handling, perhaps a default method could be added to Routable to handle route errors. By default, it does nothing, maybe it logs an error to the console. But users can override that to set the scene/child for a component if they want. Then if it is possible, a mechanism could be developed to bubble the error up to parent components until one is encountered that does override the handle_route_error method.

@@ -37,6 +69,35 @@ pub struct RouteInfo {
pub fragment: Option<String>
}

impl Iterator for RouteInfo {
Copy link
Member Author

@hgzimmerman hgzimmerman Apr 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an iterator to RouteInfo. I would love it something like this could be used to only expose relevant path information to particular components.

So the main Model would get the first segment, and then its child would get the next, etc, and would only expose the query and fragment to the leafs in the tree of components.

tune_from_part() would take a RouteSection that is generated from a mutable RouteInfo that is shared among scopes and use that to determine its child instead of a &str.

This would require that the callbacks that would call fit_to_url_path() always fire in order. Such that the main Model updates first, then its children, then so on. If this ever occurs out of order, then this iterator approach would not work, as the path segments would not necessarily correspond to the components expecting to match on them.

@hgzimmerman
Copy link
Member Author

hgzimmerman commented Apr 20, 2018

Now that I look at it, I think that there a a few ambiguities that I would like to clear up regarding what you want out of a routing service.

I'm inclined to keep throwing ideas at the wall about how routing could be done, starting with how we think what interfaces should be presented to users, and working backwards from there to see if it is possible to implement. Once we settle on a design, I'll try implementing another prototype.

  • Do you want there to be a single point of routing? Should there be one top-level component that gets the full route info, or full route String, and uses that to create a nested enum that contains the information for all of the children it has?
  • Or do you want there to be multiple components that are responsible for routing? Each component is responsible for determining what its child is, and can hook into Yew and the router to get routing information that is relevant to it.
    • If this is the choice, does the whole RouteInfo get passed to the component? If that is the case, then deriving static routes would be more difficult, as the path section index that each component corresponds to would have to be specified by the crate user.
  • Do you want to use unstructured strings to represent the path, query, and fragment, or are you in favor of the RouteInfo approach.
    • If you want to use the RouteInfo, are you in favor of further segmenting it and presenting only sections of it at a time to users, or should they have access to the whole thing, and have them be responsible for determining what section(s) they want to route with?
  • You mentioned that you want to eventually decouple Yew from its services, to move them into separate crates. By hooking into mount_in_place(), you tightly couple the router to Yew, which would make it harder to separate them in the future. Do you have a solution for this? Possibly expose some way for services to hook into the component lifecycle in a generic way?

Going in an entirely different direction...

  • I've looked at some of Yew's internals and I sort of see how you could deal with a Routable, but for the most part, it seems to beyond me, so I apologize in advance for any impossibilities I suggest here. It does give me the idea that if we are going to tightly couple the router to Yew, one avenue of doing so could be creating something like a VRoute struct, similar to VComp, that is dictated by a user implementing Routable or Router (whatever we decide to call it) on a struct. This node variant would be responsible for resolving a child VComp depending on the user's implementation of tune_from_part(). Even though my current prototype implements routing on top of a Component, I think it would be simpler from a user's experience to have routers and components be distinct from each other. So for a component, they could have a Model tied to displaying things and managing user input, and then a Router that they specify in the component's view like:
html!{
    <div>
        <div> Normal component things</div>
        <Router: />
    </div>
}

And all the `<Router: /> does is choose what the nested component would be. Because the state of a Component and what its routed child is should be orthogonal, I think they should be separated if possible.

@vitiral
Copy link
Contributor

vitiral commented Apr 21, 2018

I have several concerns about this approach. From the way I see it, a user of this approach is required to:

  • create a Route enum
  • implement trait Router with two methods for Route
  • implement Renderable for Route.

This is a lot of boilerplate which does not fit into the rest of the ecosystem (rendering IMO should only happen from the Model::view but this will somehow circumvent that?). Even worse, it has no way of converting a route into a Msg -- so you can not do more complex things like handle query strings, etc. I'm also confused how history State changes could be handled when you have no way of transmitting data via a Msg.

The approach I outlined uses a single function to accomplish the routing task and can handle all of the above concerns. If the user wants additional security they can use enums and macros to validate that their endpoints are valid html as much as they would like.

I think the router should stay out of the user's way and give them maximum flexibility. This solution gives very little flexibility requires them to use the router for only rendering a view.

/// Sets the route via the history api.
/// If the route is not already set to the string corresponding to the provided RouteInfo,
/// the history will be updated, and the routing callback will be invoked.
pub fn set_route<T: Router>(&mut self, r: T) {
Copy link
Member Author

@hgzimmerman hgzimmerman Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In response to your statement regarding this approach being too structured regarding the use of RouteInfo, I present this change that could be applied to set_route() and replace_url():
Instead of taking <T: Router>, it could instead take <T: TryInto<RouteInfo>> and TryFrom<String>, TryFrom<&str>, and From<Router> could be implemented for RouteInfo.
This would allow crate users to call set_route() with either a string type or a RouteInfo.
In the case of the conversion to RouteInfo possibly failing, an error could be printed to the console to let developers know that they formatted their route incorrectly if they are using strings.

@hgzimmerman
Copy link
Member Author

hgzimmerman commented Apr 23, 2018

I will make the argument that in order to easily resolve which page or page hierarchy to display when a user enters the site with a route already specified in the URL, the routing framework requires that the developer specify ways for a page to both set the route, as well as determine how to render itself by using the route. If that can be accomplished in a single function, I will be impressed, but as far as I can tell, your proposed solution does not facilitate the setting of the route via Yew. Your example uses <a> tags to set the fragment, which doesn't reload the page, and for that application, it works fine. But if you want to set the path using <a> tags, that will trigger a page reload (please correct me if I'm wrong about this point), which isn't desirable in a SPA. Therefore, there needs to be some framework-based means of setting the route without triggering page reloads.

I acknowledge that my approach brings about significant baggage in the form of boilerplate, but in order to address the requirements of both setting and "serializing" state via a route string, I think it is necessary and justified.

To address your other concerns:

  • Creating a Msg isn't strictly necessary, my approach sets application state by determining which child component to render via an enum variant resolved from the route, and propagates changes via the change() method instead of update() as you seem to suggest.
  • It is also possible to set up a monolithic Router at the top of your application that will set each layer of child components beneath it. It isn't strictly necessary to implement Router for every route level in your component hierarchy, although I do believe that it is a good code organization practice and encourage it in my example.
    • That monolithic router is capable of dealing with strings, as the provided RouteInfo has a to_string() method. This would allow the use of regexes or nested pattern matching on the route string to set the route enum.
  • If you would like to see the router used in a more real context, I'm currently using it in an unfinished medium sized project. It serves my needs well (although it definitely could be better), and demonstrates some flexibility that my example doesn't, like working with dynamic routes. The code is available here: https://github.com/hgzimmerman/WeekendAtJoes4/tree/master/frontend7/src . I suggest just reading it instead of running it because it relies on a backend server, and setting that up may be painful.

@hgzimmerman
Copy link
Member Author

I will have more complete thoughts about it later, but I would like to admit that I think I was wrong to prefer that the set_route() method only take a T: Router, and that a String would be a better, more flexible parameter.

For the current implementation, I think that the Router trait is a good idea, but I don't think its use should be required by Yew itself.

I've used this router to build what is currently a 5kloc app, and will continue to make changes to this branch to iron-out edge cases. Feedback and discussion are still appreciated.

bors bot added a commit that referenced this pull request Jun 16, 2018
272: Multi-threading, concurrency, agents r=DenisKolodin a=DenisKolodin

This is a series of bold experiments and I really love 💓 this PR.
It makes this framework a **multi-threaded** (it's not a joke) and brings actors model everywhere.
Now your yew frontend-apps will be more _Erlang_ or _Actix_ apps like 🚀

Also, I've removed a context. Completely! Components simplified. Now it's an actor which you could connect to and interact with messages.
Other benefit is your components could interact each other #270

Since this PR will be merged the framework turned into multi-threaded concurrency-friendly frontend framework. Sorry me for buzzwords overload )

It still need Routing #187 and fixes of the most issues. I'll get to that.
But extra benefit of this PR: it fixes major emscripten issues #220

Remaining:

- [x] Add CHANGELOG.md
- [x] Update README.md
- [x] Create issue: Send `Connected` notification for `Private` agents (#282)
- [x] Create issue: Send `Connected` notification for `Public` agents (#282)
- [x] Create issue: Implement `Global` kind of agents (based on `SharedWorker`) (#283)
- [x] Create issue: Add components interaction example (#284)

Co-authored-by: Denis Kolodin <deniskolodin@gmail.com>
@hgzimmerman
Copy link
Member Author

I'm going to dive back into this in the coming days. I haven't played around with Yew 0.6 features (actor update) yet, but I think the recent changes map very well to what I want to accomplish with routing.

  • I think the routing service should be simple, dealing with strings as its inputs and outputs. This will ship as another one of Yew's services.

  • A more strictly defined system would be implemented as part of a Worker that components that care about routing could link to. This Worker would appear only in the example code and wouldn't ship with the library itself.

    • This way, the library endorses what I hope to be a reasonable full routing solution, but the primitive routing system is kept simple and flexible so that people can arrive at better or more relevant solutions over time without involving breaking changes to the library API.
  • The current routing prototype involves getting a string from the location service, parsing it into an intermediate representation of path, query, and fragment strings and then letting a series of developer-defied enums try to create variants of themselves from these strings in a hierarchical fashion, mutably consuming path strings along the way, and only exposing the query and fragment strings to the last element. This obviously has problems; like what if the component that deals with the bar sub-path in url: .../foo/bar/baz?index=3 is responsible for pagination using the index query string?

  • The newer routing system will involve any component interested in routing linking to a Router (Worker?) actor. This involves setting a callback in create() that will receive messages from the Router and forward them to the component's update() function so that the developer can determine how to route itself. The component will also have to pull path, query and fragment information from the Router in create() as well. Because this information will be available all the time, this should remove the necessity of mutation of RouteInfo and allows any part of the route to be accessed by a routed component. This has the drawback of causing the "routing schema" to be more loosely defined, but I think that could be mitigated if one so chooses.

Hopefully, I'll have something like this ready in a couple of days.

bors bot added a commit that referenced this pull request Jun 26, 2018
296: Routing service + Router Actor example r=DenisKolodin a=hgzimmerman

Here is a new routing implementation to replace the one proposed in #187.

It creates a bare-minimum routing service around which an opinionated Router Agent is constructed and then used in a new example.

Co-authored-by: Henry Zimmerman <zimhen7@gmail.com>
@hgzimmerman
Copy link
Member Author

Since was #296 merged, this can be closed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants